deps: make the sqlx TLS backend selectable - #967
Conversation
|
Yeah, I think the rationale makes sense, but there are a few cases that need to be fixed before this is ready:
There is also one compatibility question. Today, I think the no-provider configuration should fail at compile time instead of silently building ETL without TLS. For #[cfg(not(any(
feature = "tls-rustls-ring",
feature = "tls-rustls-aws-lc-rs",
)))]
compile_error!(
"Either `tls-rustls-ring` or `tls-rustls-aws-lc-rs` must be enabled."
);Direct #[cfg(all(
feature = "store",
not(any(
feature = "tls-rustls-ring",
feature = "tls-rustls-aws-lc-rs",
)),
))]
compile_error!(
"Either `tls-rustls-ring` or `tls-rustls-aws-lc-rs` must be enabled when `store` is enabled."
);This would be a breaking change for users with @iambriccardo, wdyt? Is requiring those users to select a TLS provider an acceptable breaking change? If yes, I think the compile-time checks plus documenting the required feature are the safest way to handle it. If not, I don't think this design can both preserve the old behavior and support an AWS-LC-only build. |
|
I think this is a fair change to do. We are currently in alpha phase, so breaking changes are expected. |
farazdagi
left a comment
There was a problem hiding this comment.
So, please do the changes I asked in the comment, and we will take it from there.
sqlx's tls-rustls feature is an alias for tls-rustls-ring, so every crate here links ring with no way for a consumer to choose otherwise. sqlx also prefers ring when both backends are enabled, so a consumer cannot override it by adding a feature: the choice has to be made where the dependency is declared. The library crates now expose tls-rustls-ring and tls-rustls-aws-lc-rs, defaulting to ring so nothing changes for existing users. Binaries keep ring explicitly. This lets a consumer that already uses aws-lc-rs elsewhere avoid linking two cryptographic implementations; etl-postgres is itself such a case, since it declares rustls with the aws-lc-rs feature while its sqlx pulls ring. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
… one Each library crate now forwards its tls-rustls-ring or tls-rustls-aws-lc-rs feature to the ETL crates it depends on, so selecting a backend on etl-destinations or etl-maintenance also selects it on etl and etl-postgres. etl-maintenance replaces its hardcoded ring feature with the same selectable pair. etl and etl-postgres fail at compile time when neither backend is enabled, so a default-features = false build cannot silently drop TLS. The etl-postgres check is unconditional because sqlx is a required dependency used by the source, slots, lag, and store modules. Binaries, examples, and the fuzz crate select ring explicitly to preserve current behavior. A new CI job builds the library crates with only AWS-LC and verifies that the sqlx ring feature is absent from the resolved feature graph. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
d0b28af to
dc5ffdf
Compare
|
@farazdagi sorry for the delay, please have another look at the PR which now should include your changes requested |
sqlx's tls-rustls feature is an alias for tls-rustls-ring, so every crate here links ring with no way for a consumer to choose otherwise. sqlx also prefers ring when both backends are enabled, so a consumer cannot override it by adding a feature: the choice has to be made where the dependency is declared.
The library crates now expose tls-rustls-ring and tls-rustls-aws-lc-rs, defaulting to ring so nothing changes for existing users. Binaries keep ring explicitly. This lets a consumer that already uses aws-lc-rs elsewhere avoid linking two cryptographic implementations; etl-postgres is itself such a case, since it declares rustls with the aws-lc-rs feature while its sqlx pulls ring.
What kind of change does this PR introduce?
Build feature
What is the current behavior?
Two crypto implementations being pulled in as opposed to one
What is the new behavior?
Configurable build to use aws-lc-rs everywhere